Skip to content

Fix GH-11086: FPM: config test runs twice in daemonised mode #13357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

bukka
Copy link
Member

@bukka bukka commented Feb 8, 2024

The previous check for STDERR did not work because it never actually gets STDERR_FILENO but new file descriptor which is greater than 2. Unfortunately I did not come up with a better way than checking the source file name which might not be always reliable and cover all case - specifically when there is a link to stderr. We could probably check a link (readlink) in such case and compare but it seems quite unlikely that anyone would use that - we can do it if anyone complains. I have been also trying isatty but that does not work if FPM is executed by script (specifically it does not work in our test env).

The previous check for STDERR did not work so this fixes it
@bukka
Copy link
Member Author

bukka commented Feb 8, 2024

I'm targeting just master as duplicated logging is not such a big issue and some users might even relay on it. It is also something that I found so it might not even bother anyone.

Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Unfortunately I did not come up with a better way than checking the source file name which might not be always reliable and cover all case - specifically when there is a link to stderr. We could probably check a link (readlink) in such case and compare but it seems quite unlikely that anyone would use that - we can do it if anyone complains

I agree that just looking at the filename is probably enough in this context

@bukka bukka closed this in a19267d Mar 9, 2024
@bukka
Copy link
Member Author

bukka commented Mar 9, 2024

I merged it PHP-8.3 as well at the end as I think it's a low risk and it's a bug. Not that important for 8.2 though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants